ref(cleanup): Minor changes #101558
ref(cleanup): Minor changes #101558
Conversation
Changes included: * Start transaction for each process * Add metric to track process errors
github-actions
bot
added
the
Scope: Backend
|
|
||
| return | ||
|
|
||
| model_name, chunk = j |
There was a problem hiding this comment.
This section is easier to review by hiding the white spaces:
https://github.com/getsentry/sentry/pull/101558/files?diff=split&w=1
| logger.exception("Error in multiprocess_worker.") | ||
| finally: | ||
| task_queue.task_done() | ||
| with sentry_sdk.start_transaction(op="cleanup", name="multiprocess_worker") as transaction: |
There was a problem hiding this comment.
All the errors from the processes are missing the transaction:
| finally: | ||
| task_queue.task_done() | ||
| with sentry_sdk.start_transaction(op="cleanup", name="multiprocess_worker") as transaction: | ||
| transaction.set_tag("model", model_name) |
There was a problem hiding this comment.
This will help me distinguish the impact per model.
| if not task.chunk(apply_filter=True): | ||
| break | ||
| except Exception: | ||
| metrics.incr("cleanup.error", instance=model_name) |
There was a problem hiding this comment.
Eventually we can create monitors for this. We should not let things get as bad as they currently are.
| is_flag=True, | ||
| help="Send the duration of this command to internal metrics.", | ||
| hidden=True, | ||
| help="(deprecated) Send the duration of this command to internal metrics.", |
There was a problem hiding this comment.
All scripts are passing the --timed parameter, thus, we don't need this.
| # main process tracks the overall cleanup operation performance. | ||
| with sentry_sdk.start_transaction(op="cleanup", name="cleanup") as transaction: | ||
| transaction.set_tag("router", router) | ||
| transaction.set_tag("model", model) |
There was a problem hiding this comment.
All errors are happening within the processes, thus, these taggings are useless.
I will remove the other one on a different commit/PR to save triggering CI.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-actions
bot
added
the
Scope: Frontend
ryan953
removed
the
Scope: Frontend
| if not task.chunk(apply_filter=True): | ||
| break | ||
| except Exception: | ||
| metrics.incr("cleanup.error", instance=model_name) |
There was a problem hiding this comment.
You might want a sample_rate=1.0 on this as our default sampling rate of 10% could mean no metrics.
| model: tuple[str, ...], | ||
| router: str | None, | ||
| timed: bool, | ||
| _: bool, |
There was a problem hiding this comment.
Why change the name of this parameter? Click uses parameter names to pass CLI flags in.
There was a problem hiding this comment.
I renamed it since it was not being used. I didn't realized of the importance of it. I can verify that it would have broken the script. Thank you.
| model: tuple[str, ...], | ||
| router: str | None, | ||
| timed: bool, | ||
| _: bool, |
There was a problem hiding this comment.
I renamed it since it was not being used. I didn't realized of the importance of it. I can verify that it would have broken the script. Thank you.
| if not task.chunk(apply_filter=True): | ||
| break | ||
| except Exception: | ||
| metrics.incr("cleanup.error", instance=model_name) |
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues:
|
Changes included: